Skip to content

Adding invalidateCredentials() to OAuthClientProvider #570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 14, 2025
Merged

Adding invalidateCredentials() to OAuthClientProvider #570

merged 2 commits into from
Jul 14, 2025
+508 −83

Conversation

@geelen
Copy link
Contributor

@geelen geelen commented May 30, 2025

From working on mcp-remote, I'm seeing a lot of cases where the local state and the server state drift apart. It's especially common when iterating locally (see geelen/mcp-remote#36), but seems to be happening with production servers too.

The issue was that while the SDK defines a series of specific OAuthError types, they're only used on the server side of things. At the crucial point, where POST /token is being called and a invalid_client or invalid_grant are being received, the client simply logs that the request failed and continues: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/client/auth.ts#L166

This PR addresses this by looking for those particular error codes and invoking a new method on the OAuthClientProvider (if present): invalidateCredentials. This takes an argument of 'all' | 'client' | 'tokens' | 'verifier', but currently only all and tokens are used.

How Has This Been Tested?

Some tests have been added (generated by Amp, I'm not entirely happy with them but ran out of time and wanted to submit for feedback).

mcp-remote has been released in preview under https://pkg.pr.new/mcp-remote@96 with these changes and has confirmed that the errors in geelen/mcp-remote#36 are fixed.

Breaking Changes

Any error other than invalid_client or invalid_grant are now re-thrown, rather than silently swallowed. But those errors were likely unrecoverable anyway so this would arguably just change the kind of crash message.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Marked as draft as main has moved on so merging isn't possible yet. Would love reviews on the approach though. I will clean up and merge next week.

This was referenced May 30, 2025
Merged
Open
@pcarleton
Copy link
Member

I like this idea! a few notes on the approach:

  • the nested try/catch + passing the error in as an arg is a bit tough to follow, might be time to refactor that method into some sub-methods. put an alternative structure below, lmkwyt
  • if you can add an example in the examples repo, that'd be great to show what you expect folks to do in the invalidate method.

alternative to avoid recursion + nested try/catch:

  type AuthSuccess = {
    success: true;
    result: AuthResult;
  };

  type AuthError = {
    success: false;
    error: Error;
  };

  type AuthResponse = AuthSuccess | AuthError;

  export async function auth(
    provider: OAuthClientProvider,
    options: { serverUrl: string | URL, authorizationCode?: string },
  ): Promise<AuthResult> {
    const response = await authInternal(provider, options);

    if (response.success) {
      return response.result;
    }

    // Handle specific error types
    if (response.error instanceof InvalidClientError ||
         response.error instanceof UnauthorizedClientError) {
      await provider.invalidateCredentials?.('all');
      const retryResponse = await authInternal(provider, options);
      if (retryResponse.success) {
        return retryResponse.result;
      }
    } else if (response.error instanceof InvalidGrantError) {
      await provider.invalidateCredentials?.('tokens');
      const retryResponse = await authInternal(provider, options);
      if (retryResponse.success) {
        return retryResponse.result;
      }
    }

    // Couldn't recover
    throw response.error;
  }

  async function authInternal(
    provider: OAuthClientProvider,
    options: { serverUrl: string | URL, authorizationCode?: string },
  ): Promise<AuthResponse> {
    try {
      // Normal auth flow implementation
      // ...existing implementation...

      return { success: true, result: resultValue };
    } catch (error) {
      return {
        success: false,
        error: error instanceof Error ? error : new Error(String(error)),
      };
    }
  }

@geelen geelen force-pushed the invalidate-credentials branch 2 times, most recently from 3d1aaa6 to ecb41f5 Compare June 6, 2025 08:26
@geelen geelen marked this pull request as ready for review June 6, 2025 08:32
@geelen
Copy link
Contributor Author

geelen commented Jun 6, 2025

Updated. Available for testing on https://pkg.pr.new/geelen/typescript-sdk/@modelcontextprotocol/sdk@cdf3508

I'm going to cut an mcp-remote release using this now, since it seems to make a big difference to have any invalid data automatically cleaned

geelen added a commit to geelen/mcp-remote that referenced this pull request Jun 6, 2025
@geelen
Implementing invalidateCredentials
c7a376a 
See modelcontextprotocol/typescript-sdk#570
geelen added a commit to geelen/mcp-remote that referenced this pull request Jun 6, 2025
@geelen
Implementing invalidateCredentials
b366dec 
See modelcontextprotocol/typescript-sdk#570
geelen added a commit to geelen/mcp-remote that referenced this pull request Jun 6, 2025
@geelen
Implementing invalidateCredentials (#96)
36ed05e 
See modelcontextprotocol/typescript-sdk#570
pcarleton
pcarleton reviewed Jun 10, 2025
View reviewed changes
Copy link
Member

@pcarleton pcarleton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me, one open question about the double errorCode declaration, and I'm wondering if there's a more idiomatic way to do that. I'll try to chase that down and if we don't come up with anything, merge as is.

@geelen geelen force-pushed the invalidate-credentials branch from ecb41f5 to 4352f69 Compare June 12, 2025 00:52
@geelen
Copy link
Contributor Author

geelen commented Jun 12, 2025

Rebased and tweaked based on feedback above, should be good to go

pcarleton
pcarleton requested changes Jun 20, 2025
View reviewed changes
This was referenced Jun 20, 2025
Open
Closed
@localden localden mentioned this pull request Jun 24, 2025
Merged
@ihrpr ihrpr added this to the auth milestone Jun 25, 2025
@bpedman bpedman mentioned this pull request Jul 1, 2025
Closed
@geelen geelen force-pushed the invalidate-credentials branch from 4352f69 to 0f4b666 Compare July 4, 2025 05:30
@geelen
Refactoring OAuthErrors
e36695b 
This makes it possible to parse them from JSON, using OAUTH_ERRORS

Invalidating credentials & retrying when server OAuth errors occur

Updated existing tests

Added some initial test coverage

refactored to avoid recursion as recommended
@geelen geelen force-pushed the invalidate-credentials branch from 0f4b666 to e36695b Compare July 7, 2025 00:54
@geelen geelen requested a review from pcarleton July 7, 2025 01:12
@geelen geelen mentioned this pull request Jul 7, 2025
Merged
@pcarleton pcarleton merged commit 4b2ad03 into modelcontextprotocol:main Jul 14, 2025
2 checks passed
chenxi-null pushed a commit to chenxi-null/typescript-sdk that referenced this pull request Jul 28, 2025
@geelen @pcarleton @chenxi-null
Adding invalidateCredentials() to OAuthClientProvider (modelconte…
9a31303 
…xtprotocol#570)

Co-authored-by: Paul Carleton <[email protected]>
@ChaskyH ChaskyH mentioned this pull request Aug 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcarleton pcarleton pcarleton approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

auth

Development

Successfully merging this pull request may close these issues.

3 participants

@geelen @pcarleton @ihrpr